Add E2E test for the TLSAdherence openshift/api field#31309
Add E2E test for the TLSAdherence openshift/api field#31309richardsonnick wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughNew extended apiserver tests add TLSAdherence feature-gate gating, validate ChangesTLSAdherence Integration Tests
TLS Minimum Versions Context Setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: richardsonnick The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/apiserver/tls_adherence.go`:
- Around line 120-131: The test assertion in the It block does not account for
the optional spec.tlsAdherence field being unset. Since the field is marked as
optional with omitempty and the test comment acknowledges "when set", the
apiServer.Spec.TLSAdherence value may be nil, causing the BeElementOf assertion
to fail even when the state is valid. Either guard the assertion with a nil
check that conditionally validates only when the field is set, or add the
nil/zero value to the validValues slice so that an unset field is considered a
valid state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 328d6dfb-fe5e-4dfc-a5f3-811d6c04d0e2
📒 Files selected for processing (1)
test/extended/apiserver/tls_adherence.go
| g.It("[FeatureGate:TLSAdherence] should have TLSAdherence listed as enabled in featuregate/cluster status [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| fg, err := oc.AdminConfigClient().ConfigV1().FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get featuregate/cluster") | ||
|
|
||
| found := false | ||
| for _, featureGateValues := range fg.Status.FeatureGates { | ||
| for _, enabledGate := range featureGateValues.Enabled { | ||
| if enabledGate.Name == tlsAdherenceFeatureGateName { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if found { | ||
| break | ||
| } | ||
| } | ||
| o.Expect(found).To(o.BeTrue(), | ||
| "TLSAdherence must appear in featuregate/cluster .status.featureGates[].enabled[].name") | ||
| }) |
There was a problem hiding this comment.
Can't hurt to verify this, but I don't think this should count toward the 5 tests, the intent of which are more functional rather than verifying assumptions.
| // Test 5 – verify that no cluster operator is degraded when the TLSAdherence feature gate is active. | ||
| g.It("[FeatureGate:TLSAdherence] should not have any degraded cluster operators [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| coList, err := oc.AdminConfigClient().ConfigV1().ClusterOperators().List(ctx, metav1.ListOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to list clusteroperators") | ||
|
|
||
| for _, co := range coList.Items { | ||
| for _, condition := range co.Status.Conditions { | ||
| if condition.Type == configv1.OperatorDegraded && condition.Status == configv1.ConditionTrue { | ||
| g.Fail(fmt.Sprintf("cluster operator %q is degraded: %s: %s", | ||
| co.Name, condition.Reason, condition.Message)) | ||
| } | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
I think there is a different kind of test that we can implement that essentially continuously monitors the cluster to verify an invariant is never violated. The way this test is implemented, you'll only get a point in time verification, which would likely result in false negatives.
|
Scheduling required tests: |
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
richardsonnick
force-pushed
the
tls-adherence-e2e
branch
from
e65c04c to
14d023a
Compare
|
Scheduling required tests: |
richardsonnick
force-pushed
the
tls-adherence-e2e
branch
from
14d023a to
f72bc40
Compare
|
/test ? |
|
/test e2e-aws-ovn-single-node-techpreview |
|
/test e2e-gcp-ovn-techpreview |
richardsonnick
force-pushed
the
tls-adherence-e2e
branch
from
f72bc40 to
a910371
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/apiserver/tls_adherence.go (1)
198-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify Go's short-circuit case evaluation here is intentional and safe.
The
case original.Spec.TLSSecurityProfile == nil, original.Spec.TLSSecurityProfile.Type == ...:pattern relies on Go evaluating case expressions left-to-right and stopping at the first match, which prevents a nil dereference whenTLSSecurityProfileis nil. This is correct per the Go spec but is a subtle pattern worth a one-line comment for future maintainers, since it's easy to mistakenly "fix" by reordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/apiserver/tls_adherence.go` around lines 198 - 210, Add a brief inline comment in the switch over original.Spec.TLSSecurityProfile explaining that the nil check must come first because Go evaluates case expressions left-to-right, so the Type access is safe only after the nil case short-circuits; keep this near the tlsShouldWork/tlsShouldNotWork setup in tls adherence test logic so future readers do not reorder the cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/apiserver/tls_adherence.go`:
- Around line 184-196: The cleanup in the APIServer TLS adherence test does not
actually restore the documented fallback when original.Spec.TLSAdherence was
unset, so the cluster can remain at StrictAllComponents after the test. Update
the DeferCleanup logic in tls_adherence.go to explicitly set the APIServer spec
back to LegacyAdheringComponentsOnly when the original value was empty, and only
skip the update when that fallback is already present; keep the existing restore
path for the APIServer client update using original.Spec.TLSAdherence and the
APIServers().Update flow.
---
Nitpick comments:
In `@test/extended/apiserver/tls_adherence.go`:
- Around line 198-210: Add a brief inline comment in the switch over
original.Spec.TLSSecurityProfile explaining that the nil check must come first
because Go evaluates case expressions left-to-right, so the Type access is safe
only after the nil case short-circuits; keep this near the
tlsShouldWork/tlsShouldNotWork setup in tls adherence test logic so future
readers do not reorder the cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 967b6fb4-7f0d-4fb5-a7a4-7f1da3dc89df
📒 Files selected for processing (2)
test/extended/apiserver/tls.gotest/extended/apiserver/tls_adherence.go
✅ Files skipped from review due to trivial changes (1)
- test/extended/apiserver/tls.go
|
Scheduling required tests: |
Tests that apiservers/cluster spec.tlsAdherence: - rejects invalid values (422 Invalid via dry-run) - accepts and reflects all valid values (dry-run) - is only writable by cluster-admins (SubjectAccessReview) - does not leave cluster operators degraded Also labels the existing TestTLSMinimumVersions and TestTLSDefaults wire-level TLS tests with [FeatureGate:TLSAdherence] so they count toward the openshift/api verify-feature-promotion Sippy check.
richardsonnick
force-pushed
the
tls-adherence-e2e
branch
from
a910371 to
b1744bd
Compare
|
Scheduling required tests: |
|
/test ci/prow/e2e-vsphere-ovn |
|
@richardsonnick: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test ci/prow/e2e-vsphere-ovn |
|
@richardsonnick: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@richardsonnick: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Adds E2E tests for the TLSAdherence openshift/api field. This is a requirement for GA.
Summary by CodeRabbit
spec.tlsAdherencebehavior.spec.tlsAdherencevalues are rejected and valid values (StrictAllComponents,LegacyAdheringComponentsOnly) are accepted.